Skip to content

[1/N][CI] Move linting system to pre-commits hooks #1256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[1/N][CI] Move linting system to pre-commits hooks #1256

wants to merge 2 commits into from
+750 −619

Conversation

Potabk
Copy link
Contributor

@Potabk Potabk commented Jun 17, 2025 •
edited
Loading

What this PR does / why we need it?

Follow vllm-project/vllm lint way: https://github.com/vllm-project/vllm/blob/main/.pre-commit-config.yaml

Enable pre-commit to avoid some low level error AMAP.

This pr is one step of #1241, The purpose is make linting system more clear and convenient, on this step, Mainly did the following things: yapf, actionlint, ruff, typos, isort, mypy, png-lint, signoff-commit, enforce-import-regex-instead-of-re.

TODO:

  • clang-format(check for csrc with google style)
    need clean code, disable for now
  • pymarkdown
    need clean code, disable for now
  • shellcheck
    need clean code, disable for now

Does this PR introduce any user-facing change?

Only developer UX change:
https://vllm-ascend--1256.org.readthedocs.build/en/1256/developer_guide/contributing.html#run-lint-locally

pip install -r requirements-lint.txt && pre-commit install
bash format.sh

How was this patch tested?

CI passed with new added/existing test.

Co-authored-by: Yikun [email protected]
Co-authored-by: wangli [email protected]

@github-actions github-actions bot added documentation Improvements or additions to documentation ci/build module:tests module:ops module:tools and removed documentation Improvements or additions to documentation module:tests module:ops labels Jun 17, 2025
@Potabk Potabk force-pushed the lint-2 branch 2 times, most recently from 3313968 to b6b8bf3 Compare June 17, 2025 09:08
@github-actions github-actions bot added documentation Improvements or additions to documentation module:tests module:ops labels Jun 17, 2025
@Potabk Potabk force-pushed the lint-2 branch 2 times, most recently from 0ff0fa9 to e196c98 Compare June 18, 2025 01:35
@Potabk
Copy link
Contributor Author

Potabk commented Jun 18, 2025

cc @Yikun @wangxiyuan

@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@Yikun
Copy link
Collaborator

Yikun commented Jun 19, 2025

c8bb90f

  • Update format.sh to have two cmd format.sh and format.sh ci
  • Update the vllm-ascend doc
  • Add some note

After consideration, I didn't add mypy lint to format.sh, but added format.sh ci

@Yikun
Copy link
Collaborator

Yikun commented Jun 19, 2025

Need a rebase after #1293 resolved

Yikun
Yikun reviewed Jun 19, 2025
View reviewed changes
.github/workflows/vllm_ascend_test.yaml Outdated
@@ -18,8 +18,6 @@
name: 'test'

on:
schedule:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we remove the schedule here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, currently this is a per PR check, we can enable push trigger in future

@Yikun Yikun added the ready read for review label Jun 20, 2025
@github-actions github-actions bot added merge-conflicts and removed ready read for review labels Jun 20, 2025
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@codecov Codecov
Copy link

codecov bot commented Jun 25, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 27.41%. Comparing base (c30ddb8) to head (1ca0957).
Report is 43 commits behind head on main.

Files with missing lines Patch % Lines
vllm_ascend/ops/fused_moe.py 0.00% 4 Missing ⚠️
vllm_ascend/ops/vocab_parallel_embedding.py 0.00% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1256      +/-   ##
==========================================
+ Coverage   27.39%   27.41%   +0.02%     
==========================================
  Files          56       56              
  Lines        6191     6270      +79     
==========================================
+ Hits         1696     1719      +23     
- Misses       4495     4551      +56
Flag Coverage Δ
unittests 27.41% <0.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Potabk @Yikun
add pre-commit
9b07f5e 
Signed-off-by: wangli <[email protected]>
Signed-off-by: Yikun Jiang <[email protected]>
@Potabk
Copy link
Contributor Author

Potabk commented Jun 25, 2025 •
edited
Loading

since we have new linting sys(typos), we don't need doc_codespell

@Potabk
Copy link
Contributor Author

Potabk commented Jun 25, 2025

will enable clang-format, pymarkdown, shellcheck in the next step

del doc_codespell
1ca0957 
Signed-off-by: wangli <[email protected]>
@Yikun Yikun mentioned this pull request Jun 28, 2025
Open
40 tasks
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yikun Yikun Yikun approved these changes

@wangxiyuan wangxiyuan wangxiyuan approved these changes

Assignees
No one assigned
Labels
ci/build documentation Improvements or additions to documentation merge-conflicts module:ops module:tests module:tools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Potabk @Yikun @wangxiyuan